Skip to content

Conversation

@ensi321
Copy link
Contributor

@ensi321 ensi321 commented Jan 27, 2025

No description provided.

Comment on lines 1 to 2
Focil:
InclusionList:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll want a fork name rather than a EIP name for the api, but it's probably premature to know that I guess...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now we should keep it as EIP name (or number) but it would be good if we can merge new endpoints before those are officially part of a hard fork. I added a new tag "Draft" here to separate those in the api explorer, but maybe even a EIP specific tag would make sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In teku i just use Experimental tag but draft also works, that'd be fine...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I was using but it had a different purpose previously when it was used for the rewards apis. I think either one works, if we do this for more EIPs it might even be good to use a separate tag for each. This would allow to disable spec tests based on tag

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its been super problematic merging with feature flags sometimes, that's my hesitation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we probably won't be merging this any time soon since the CL spec isn't even merged yet but it would be nice if we can come up with an approach to allow merging this before it's confirmed as part of a hard fork.

I pushed a few changes group files / types under EIP specific namespace and used the EIP as tag as well, this would allow us to have more than one EIP in "Draft" stage and be part of the api explorer.

@nflaig nflaig changed the title Add Inclusion List endpoints Add EIP-7805 (FOCIL) endpoints Feb 4, 2025
@mehdi-aouadi mehdi-aouadi mentioned this pull request Feb 13, 2025
3 tasks
required: [data]
properties:
data:
$ref: "../../beacon-node-oapi.yaml#/components/schemas/Bellatrix.Transactions"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this meant to return EIP7805.InclusionList? maybe I'm reading things wrong but this seems to expect the transactions of the IL in the data property whereas description says this will request BN to produce an IL.

Copy link
Member

@nflaig nflaig Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depends how you define what a inclusion list is, technically it's just a list of transactions which is the return type of this api and also the engine-api

the other return type could be InclusionList (ie. wrap it in container) but this would return useless and even confusing data, eg. validator_index isn't even set by beacon node, so beacon node would just set some value (maybe 0) and then the validator client is responsible for overriding it

we can think about if we wanna wrap this in a container, and maybe even make the api fork-aware by adding version metadata, this might be better for future forward compatibility

Copy link
Contributor Author

@ensi321 ensi321 Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this meant to return EIP7805.InclusionList? maybe I'm reading things wrong but this seems to expect the transactions of the IL in the data property whereas description says this will request BN to produce an IL.

@gfukushima Yes. As @nflaig mentioned, IL can mean

  1. A list of IL transactions. It is used here, execution api spec and anything as far as EL is concerned
  2. IL container as defined in consensus specs. Basically 1 + any other useful consensus info

Here we only return IL transactions because any additional consensus info (slot, validator index, ILC root) are all available in VC. No need to grab them from BN

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants